Skip to content

refactor: replace deprecated Result.Requeue in controller reconcilers - #1348

Open
christian-heusel wants to merge 2 commits into
kubeflow:notebooks-v2from
christian-heusel:chore/fix-requeue-deprecation
Open

refactor: replace deprecated Result.Requeue in controller reconcilers#1348
christian-heusel wants to merge 2 commits into
kubeflow:notebooks-v2from
christian-heusel:chore/fix-requeue-deprecation

Conversation

@christian-heusel

@christian-heusel christian-heusel commented Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

controller-runtime deprecates reconcile.Result.Requeue in favor of
RequeueAfter / returning an error. The dependency bump in #1306 moves
the controller to controller-runtime v0.24.1, where staticcheck
flags every Result{Requeue: true} usage as SA1019:

This setting is deprecated as it causes confusion and there is no
good reason to use it. When waiting for an external event to happen,
either the duration until it is supposed to happen or an appropriate
poll interval should be used, rather than an interval emitted by a
ratelimiter whose purpose it is to control retry on error.
Deprecated: Use `RequeueAfter` instead.

Approach

The reconciler used Requeue: true in two distinct situations, handled differently:

Situation Count Old New Rationale
IsConflict(err) retry after an update 14 return Result{Requeue: true}, nil return Result{}, err A conflict is an error; returning it lets the workqueue rate-limiter handle retry-on-error — exactly what the deprecation note says that limiter is for. Preserves the identical rate-limited/backoff behavior.
Stale cache (owned ServiceAccount not yet cached) 2 return Result{Requeue: true}, nil return Result{}, nil We already Owns(&ServiceAccount{}), so the owned-object watch will requeue us — the existing code comments already say so. No explicit requeue needed.

mergeReconcileResult() no longer inspects the deprecated .Requeue field and merges purely on RequeueAfter; its unit test was updated to match.

Options considered

Option Requeue timing Logging Notes
A. return the error (this PR) immediate, rate-limited backoff — identical to Requeue: true conflict now logged at error level by controller-runtime Matches the deprecation's stated rationale; smallest behavioral change to timing.
B. RequeueAfter: <small const> fixed delay, no exponential backoff stays quiet (returns nil) Changes retry cadence; needs a somewhat arbitrary constant.
C. //nolint:staticcheck unchanged unchanged Keeps the deprecated API; just silences the linter.

Discussion points

  1. Logging noise (main one). With option A, benign optimistic-concurrency conflicts (previously logged only at V(2) and returned as nil) now surface via controller-runtime's "Reconciler error" at error level (and bump the error metric). Is that acceptable, or do we prefer option B (RequeueAfter) to keep conflicts quiet at the cost of changing the retry cadence?
  2. Stale-cache sites. Is relying solely on the Owns(&ServiceAccount{}) watch to requeue acceptable, or should we rather keep an explicit short RequeueAfter as a safety net?
  3. Scope. Should this also convert k8s.io/apimachinery/pkg/util/httpstream → k8s.io/streaming/pkg/httpstream (the other SA1019 the bump surfaces), or keep that as a separate PR?

related: #1306

@github-project-automation github-project-automation Bot moved this to Needs Triage in Kubeflow Notebooks Aug 24, 2026
@google-oss-prow google-oss-prow Bot added do-not-merge/work-in-progress area/controller area - related to controller components labels Aug 24, 2026
@google-oss-prow

google-oss-prow Bot commented Aug 24, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andyatmiami for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow Bot added area/v2 area - version - kubeflow notebooks v2 size/M labels Aug 24, 2026
@christian-heusel

christian-heusel commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

/cc @siyuanfoundation @thesuperzapper
Would be good to get some input from you on the discussion points & options given your experience in controller design & code 🤗
As noted above, the code itself is not intended to merge (given this is a topic that I am not familiar with), I just wanted to have something as a conversation starter 😄

@siyuanfoundation

siyuanfoundation commented Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown

I think for IsConflict(err) retry after an update, it is pretty standard to do return Result{}, err, but for stale cache, "We already Owns(&ServiceAccount{}), so the owned-object watch will requeue us" is not necessarily true if the same name ServiceAccount is owned by some other controller, in that case it is better to use RequeueAfter: <small const> to have requeue guarantee. There is no need for exponential back off because stale cache is pretty common and not a write contention (you just wait)

@christian-heusel
refactor: replace deprecated Result.Requeue in controller reconcilers
99711c1 
`controller-runtime` deprecates `reconcile.Result.Requeue` in favor of
`RequeueAfter` / returning an error. The dependency bump in kubeflow#1306 moves
the controller to `controller-runtime v0.24.1`, where `staticcheck`
flags **every** `Result{Requeue: true}` usage as `SA1019`:

    This setting is deprecated as it causes confusion and there is no
    good reason to use it. When waiting for an external event to happen,
    either the duration until it is supposed to happen or an appropriate
    poll interval should be used, rather than an interval emitted by a
    ratelimiter whose purpose it is to control retry on error.
    Deprecated: Use `RequeueAfter` instead.

Therefore this change replaces every usage in the controller
reconcilers:

- Conflict-retry sites now return the conflict error so the workqueue
  rate-limiter handles retry-on-error (its documented purpose), which
  preserves the same rate-limited backoff that `Requeue: true` provided.
- Stale-cache sites (`ServiceAccount` not yet in the cache) rely on the
  `Owns(ServiceAccount)` watch to requeue, matching the existing
  comments.
- `mergeReconcileResult` drops its `.Requeue` handling and now merges
  purely on `RequeueAfter` its unit test is updated accordingly.

Tradeoff for discussion: conflict errors are now surfaced by
controller-runtime at error level rather than staying quiet at `V(2)`.

Assisted-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Christian Heusel <christian@heusel.eu>
@christian-heusel
fix: requeue with a fixed delay on stale ServiceAccount cache
58a5323 
Addresses review feedback on the stale-cache handling: relying on the
`Owns(ServiceAccount)` watch to requeue is not guaranteed, because a
ServiceAccount with the deterministic name that is owned by another
controller will never be mapped back to this Workspace. Requeue with a
small fixed `RequeueAfter` instead: a stale cache is common and is not
write contention, so no exponential backoff is needed.

Assisted-by: Qwen3.8-27B
Signed-off-by: Christian Heusel <christian@heusel.eu>
@christian-heusel
christian-heusel force-pushed the chore/fix-requeue-deprecation branch from c05d80e to 58a5323 Compare August 28, 2026 17:51
@christian-heusel
christian-heusel marked this pull request as ready for review August 28, 2026 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andyatmiami andyatmiami Awaiting requested review from andyatmiami
@thesuperzapper thesuperzapper Awaiting requested review from thesuperzapper
@siyuanfoundation siyuanfoundation Awaiting requested review from siyuanfoundation

Assignees

No one assigned

Labels

area/controller area - related to controller components area/v2 area - version - kubeflow notebooks v2 size/M

Projects

Kubeflow Notebooks
Status: Needs Triage

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@christian-heusel @siyuanfoundation